Skip to content

libraries: Add the OneBot v11 channel - #1

Open
Yos-X wants to merge 15 commits into
masterfrom
feature/channel-onebot
Open

Yos-X wants to merge 15 commits into
masterfrom
feature/channel-onebot

Conversation

@Yos-X

@Yos-X Yos-X commented Oct 10, 2026 •

Copy link
Copy Markdown
Collaborator

OneBot v11 的客户端 Channel,位于 libraries/channels/onebot。

五个提交按层推进。先是协议域与动作注册表;然后是鉴权与四种通信;接着是消息映射;再把 Channel 与实例级 API 接起来;最后是 app 集成与示例配置。

协议域不涉及传输,也不引库。含 20 种消息段,认不出的段保留原始 data;三种消息形态;CQ 编解码;四类事件,认不出的保留原始 JSON;38 个公开动作与隐藏动作,_async 和 _rate_limited 由后缀派生;结果分层。

四种通信各自独立,每实例选一种。http 的对测覆盖成功、拒绝、连不上、鉴权失败、畸形应答,另有 token 不泄漏的断言;http_post 覆盖验签、路径、账号绑定与快速操作回写;ws 按 echo 关联读回应答并读事件;ws_reverse 由对端拨号接入,并经该连接完成一次调用。另有一个运行于真实 runtime 的集成测试,覆盖实现上报消息至回复经 send_private_msg 发出。

依赖方面引入 org.java-websocket:Java-WebSocket:1.6.0,仅出现在插件模块与版本目录,plugin-sdk、internal、runtime 未改动,唯一传递依赖为 slf4j-api。

以下边界本 PR 不处理:

  • 通知、请求、心跳等非消息事件读入后丢弃。连接可承载这些事件,但对外提供方式涉及接口形态的决定,未自行确定。当前获取群成员变动、好友请求等信息只能经 OneBotApi 查询。
  • MediaAttachment 读字节抛 UnsupportedOperationException。消息中的图片与语音只是文件名或 URL,取字节需调 get_image/get_record 或按 URL 下载,两者均需传输参与。
  • preview 为空实现。OneBot 无流式输出能力,streaming 为 false。
  • 反向 ws 无法在握手期返回 401。Java-WebSocket 1.6.0 的 WebSocketServer 没有可覆写的手握钩子,只能在 onOpen 校验后按 1008 关闭。

验证命令为 ./gradlew :libraries:channels:onebot:spotlessCheck :libraries:channels:onebot:test,74 个测试通过,spotless 与 allWarningsAsErrors 均通过。

:app:test 在本分支存在的失败与本 PR 无关,分两类。一类是既有平台基线:断言写死 POSIX 的路径与错误文案,运行在 Windows 上;已在父提交用 stash 复测,失败集合一致;这批由 fix/platform-dependent-assertions 分支单独修复。另一类是上游 e7747d8 引入的:Floor.kt 把 Path.of("/dev") 写入默认保护位置清单,Windows 上该路径为无盘符的 \dev,PathCanonicalizer 无法处理,AgentFloor 构造失败导致 runtime 无法启动。该问题在 master 上同样存在,属产品代码的平台缺陷,另行处理。

Yos-X added 5 commits October 10, 2026 20:02
The OneBot v11 standard, in Kotlin types and nothing else: the ids and statuses, the twenty message segment types with the unknown ones kept whole, a message in each of the three shapes the API takes, the CQ code format, the events of the four kinds plus an unknown one that keeps its JSON, the 38 public actions with the hidden one and the derived async and rate limited calls, and the layered result model. No transport and no library is involved, so the domain is testable on its own.

The module is discovered by AlexandriteLayout as channels.onebot, which gives it the built-in KSP index, the config root channels.onebot and the plugin id alexandrite-channel-onebot.
Four things a review of the standard found missing, each with the test that
would have said so:

- The HTTP transport sent its token only in the `Authorization` header,
  while the standard also allows `?access_token=`, and an implementation
  that reads only the query refused every call. It sends both now.
- An instance that serves one account accepted a reverse connection and an
  HTTP report that named no account at all, because only a value that was
  both present and different was refused. The account is what binds a
  connection to an instance, so a missing one is refused like a wrong one.
  The body of an HTTP report is read before that, so a report that is no
  event is still told apart from one of another account.
- The reverse listener read no `X-Client-Role`, so an `API` client, which
  sends no events, and an `Event` client, which answers no call, were taken
  as if they carried both. The roles that carry both are served, the two
  that do not are closed with their reason, and an implementation that names
  no role keeps working, since older ones report none.
- A port of 0 was documented as the operating system's choice and refused by
  the configuration, so the two now agree.

Verified: :libraries:channels:onebot:test passes, 74 tests.
In: a private chat is private:<user id>, a group is group:<group id>, a temporary session keeps the group as its chat and the sender as its thread, media becomes attachments, a reply becomes a quote, and a segment this version has no shape for becomes a display fact rather than being dropped. Out: plain text, Markdown images as image segments, and a reply leading with the reply segment. A failure of the implementation becomes a delivery failure the SDK knows.
OneBotLink is the one place that picks the transport of an instance and opens it, so the channel calls an action without knowing what carries it. OneBotChannel contributes itself under the channel type onebot and turns an outgoing message into the parameters of send_private_msg or send_group_msg. OneBotInbox takes a reported message into the agent as a submission of its chat. OneBotApi is the typed face of the 38 actions plus the hidden one and a raw escape, one instance of it per channel instance. call takes a suffix, so the derived _async and _rate_limited calls are reachable without giving up the request type, and a result this version cannot read keeps the answer it came from.

An integration test runs the plugin inside a real runtime through the testkit harness: the implementation reports a message, the agent is asked for a turn of that chat, and the answer goes back out through send_private_msg.
app depends on the channel, so the index the built-in list names is on the class path, and the example config documents an instance with the channel off by default. This makes the built-in runtime tests see the plugin that was listed but missing before.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Transport result handling, HTTP POST operation, temporary-session routing, and receiver resource limits contain correctness and security defects.

12 open findings
What changed in this PR

Adds a OneBot v11 channel with protocol models, authenticated HTTP/WebSocket transports, runtime mapping, typed APIs, tests, and app integration.

Changes:

  • Implements OneBot messages, events, actions, results, and authentication.
  • Adds HTTP, HTTP POST, forward WebSocket, and reverse WebSocket communication.
  • Integrates the channel into the runtime and example configuration with broad tests.
File Description
app/​build.gradle.kts Adds the OneBot module.
app/​src/​dist/​config/​alexandrite.example.json Adds example OneBot configuration.
gradle/​libs.versions.toml Registers Java-WebSocket.
libraries/​channels/​onebot/​build.gradle.kts Configures module dependencies.
OneBotPlugin.kt Declares the plugin.
api/​OneBotApi.kt Defines the public typed API.
api/​OneBotApiImpl.kt Implements API dispatch and decoding.
auth/​OneBotAuth.kt Implements tokens, signatures, and redaction.
channel/​OneBotChannel.kt Implements channel delivery.
channel/​OneBotInbox.kt Submits incoming messages.
channel/​OneBotLink.kt Selects and manages transports.
config/​OneBotConfig.kt Defines plugin and instance configuration.
mapping/​OneBotMessages.kt Maps SDK and OneBot messages.
protocol/​OneBotIds.kt Defines typed identifiers.
protocol/​OneBotStatus.kt Defines statuses and return codes.
protocol/​api/​OneBotAction.kt Models actions and suffixes.
protocol/​api/​OneBotApiTypes.kt Defines typed response payloads.
protocol/​api/​OneBotRegistry.kt Registers standard actions.
protocol/​api/​OneBotRequests.kt Defines request payloads.
protocol/​event/​OneBotEvent.kt Models OneBot events.
protocol/​event/​OneBotEventCodec.kt Decodes event objects.
protocol/​message/​CqCode.kt Implements CQ encoding.
protocol/​message/​OneBotMessage.kt Models message shapes.
protocol/​message/​OneBotSegment.kt Models message segments.
protocol/​message/​OneBotSegmentCodec.kt Encodes and decodes segments.
protocol/​result/​OneBotResult.kt Models layered call results.
transport/​Connection.kt Provides shared connection state.
transport/​OneBotForwardWebSocket.kt Implements forward WebSocket transport.
transport/​OneBotHttpApi.kt Implements HTTP API calls.
transport/​OneBotHttpPostReceiver.kt Receives HTTP event reports.
transport/​OneBotReverseWebSocket.kt Implements reverse WebSocket transport.
transport/​OneBotSettings.kt Builds transport settings.
transport/​OneBotWire.kt Configures wire JSON handling.
OneBotChannelIntegrationTest.kt Tests runtime message round trips.
auth/​OneBotAuthTest.kt Tests authentication behavior.
channel/​OneBotHttpTransportTest.kt Tests HTTP calls and failures.
mapping/​OneBotMessagesTest.kt Tests SDK message mapping.
protocol/​api/​OneBotRegistryTest.kt Tests action registration.
protocol/​event/​OneBotEventCodecTest.kt Tests event decoding.
protocol/​message/​CqCodeTest.kt Tests CQ and segment codecs.
transport/​OneBotHttpPostReceiverTest.kt Tests report reception.
transport/​WsTransportTest.kt Tests both WebSocket modes.

🧠 Review effort: Balanced


💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

A review of the pull request found twelve things. Five are defects and are
fixed here, with the test that would have caught each.

The account header of an HTTP report was looked up as the standard spells it,
while the head reader folds header names to lower case. The header was
therefore never read, and a report that named its account only there was
refused. A test that passed for the wrong reason covered it: its body named
the account too, so the missing lookup was invisible. That case now sends a
body that names none, and a second case covers the header alone.

An answer that came back over a socket was wrapped as data without reading
its status, so a `failed` answer was an `Ok` whose data was the envelope, and
a caller read a delivery that never happened. Socket answers now go through
the same classification as HTTP ones.

The echo counter was a plain long read and written from more than one caller,
so two calls could take the same echo, overwrite each other in the waiting
map, and leave one caller waiting until its timeout. It counts atomically.

The declared length of an HTTP report was used to allocate before it was
checked, so a client could ask the listener for an array of the size it
named; the later check also measured characters rather than bytes. The length
is checked against the limit before the allocation.

The remaining findings are recorded rather than fixed here: an `http_post`
instance has no endpoint to call, so its outgoing actions are unreachable and
the configuration does not let it name one; the data of an answer is read as
an object, so actions such as `get_friend_list` that answer with an array do
not fit their type; a temporary session replies to its group instead of its
sender; `autoEscape` cannot preserve CQ text because every message is sent as
an array; an unknown `message_type` is stored in `postType`; `rateLimit` and
`logUnreadableAnswers` are configured but unused; and a dropped event is not
counted.

Verified: :libraries:channels:onebot:test passes, 75 tests.
@Yos-X

Yos-X commented Oct 10, 2026 •

Copy link
Copy Markdown
Collaborator Author

已按评审处理,逐条说明对应提交。

  • X-Self-ID 大小写:请求头在读 head 时已折成小写,查询却用标准拼写,因此该头从未被读到——已修复 6469a4c。原先那条"缺少账号则拒绝"的用例是靠这个缺陷才通过的,现改为报文不带账号,另加一条只靠请求头命名账号的用例。
  • 通过 socket 返回的应答按应答处理:已修复 6469a4c。正向与反向 WebSocket 的应答与 HTTP 应答走同一套状态分类。
  • echo 计数器非原子:已修复 6469a4c。
  • 声明长度超限的报文不返回状态:已修复 f5ac753。分配前比较上限,并返回 413;声明的那点字节以 8 KiB 为块读出丢弃,使仍在发送的客户端能读到该响应。
  • 临时会话的回复发到群:已修复 951cbaf。新增 OneBotMessages.privateTemporary,action 与 params 都先判断临时会话。
  • autoEscape 无法保留 CQ 原文:已修复 951cbaf。消息按写入时的形状发送,auto_escape 只由"整条消息是一段文本"这一种形状请求。
  • 应答的 data 按对象读取,返回数组的动作对不上类型:已修复 371ffb5。data 按原形状保留,Ok 的类型参数放宽为 JsonElement,补了数组与对象两条用例。
  • 未知 message_type 被写入 postType:已修复 371ffb5。
  • 两个配置项可配置但未被消费:已修复 371ffb5,采用删除。
  • 事件溢出丢弃没有计数:已修复 371ffb5。trySend 在两种丢弃策略下都返回成功,因此计数由"已报告减已读减容量"得出。
  • 反向连接关闭时挂起的调用不结束:已修复 ee04a72。

未处理的最后一条:http_post 实例发出的动作一律 unreachable。这一条不是实现缺陷,而是该传输本身的形状。标准里的 http_post 是事件推送通道:实现方作为 HTTP 客户端把事件 POST 到本实例,本实例应答后连接即结束,因此没有可供回程的连接,本实例也没有指向实现方的地址。发出调用需要指向实现方的连接,而这条传输不提供,标准也没有要求它提供。标准另在 api 段规定了实现方自己的 HTTP 服务端地址,所以需要回程时应当配置一个 http 或 ws 实例,与 http_post 实例并存,共用同一账号。

因此这里有一个选择,属于配置面的决定:保持现状,用一个 http_post 实例收事件、另一个 http 或 ws 实例发调用;或者为 http_post 增加一个可选 endpoint,配了就同时建立 HTTP API 客户端,一个实例完成收发。该配置项尚未添加。

验证::libraries:channels:onebot:test 通过,82 个测试。

Yos-X added 4 commits October 10, 2026 22:22
Three findings of the review that were deferred, and should not have been.

A temporary session of a group is a private message whose address keeps the
group as its chat and the sender as its thread, and the mapping already said
that a reply goes back to the sender. The channel ignored the thread and
picked `send_group_msg` for the group, so an answer to a private message was
posted into the group. `privateTemporary` names the sender of such a session
and both the action and the parameters ask for it first.

Every message was sent as an array of segments, which is a shape that cannot
be taken literally, so `autoEscape` asked an implementation to preserve CQ
text and then handed it the text already cut into segments. A message is sent
in the shape it was written in, and `auto_escape` is asked for only by a
message that is one piece of text, the only shape an implementation can take
as written.

Verified: :libraries:channels:onebot:test passes, 77 tests.
A review found that the data of an answer was read as an object, so an answer
that carries an array or nothing became an empty object. The standard answers
`get_friend_list` and `get_group_list` with arrays, so every call of those was
reported as one this version cannot read, and the typed action layer had
nothing to deserialize.

The answer now keeps its data as it arrived, and the object an implementation
answers a send with is read where it is rather than where it was assumed to
be. Tests cover an array, an object, and a report that arrives while nothing
reads it, which is counted from what was taken less what was read and what
still waits, because the queue answers that it took an event even when it
displaced one.

The `rateLimitIntervalMillis` and `logUnreadableAnswers` options are removed
rather than left in place unused: this plugin does not rate limit calls and
does not log the answers it cannot read, and an option that promises either
is worse than no option.

Also corrected: an unknown `message_type` was stored in `postType`, which the
property promises is the top-level `post_type`. The subtype stays in the raw
JSON. One existing test asserted the wrong value and now asserts the right
one.

Verified: :libraries:channels:onebot:test passes, 80 tests.
…eaves

A call of the reverse transport waited for its answer under its own timeout
even after the implementation that owed it had gone, so a caller was told
that a slow implementation had missed a deadline rather than that the
connection was lost. A close now ends every call that waits, with the reason
the connection gave, and a test drives a call that is never answered into a
close and reads the failure.

Verified: :libraries:channels:onebot:test passes, 81 tests.
A report whose declared length was over the limit was refused by reading it
as no report at all, which the implementation saw as a connection that
dropped rather than as a refusal it could act on. The length is still checked
before anything is allocated, and what the length declares is now read in
pieces and dropped so that the answer reaches a peer that is still sending
it. The piece size is what keeps the near-2 GiB allocation out.

Verified: :libraries:channels:onebot:test passes, 82 tests.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Transport error handling, reverse-WebSocket isolation, event accounting, serialization, and configuration contain unresolved correctness and security issues.

5 open findings
12 resolved since last review
Previously missed (10)

In code that hasn't changed since last review

Medium severity Convert WebSocket transport failures to Unreachable results

libraries/​channels/​onebot/​src/​main/​kotlin/​org/​foedusprogramme/​alexandrite/​channel/​onebot/​channel/​OneBotLink.kt:79

WebSocket transport failures escape from this method instead of becoming OneBotResult.Unreachable. A disconnected socket throws IllegalStateException, and the per-call timeout throws TimeoutCancellationException, so both Channel.send and the public typed API can throw while the HTTP path returns a result. Convert transport-owned timeout/connection failures here while still rethrowing caller cancellation.

Medium severity Validate numeric reply IDs before constructing MessageId

libraries/​channels/​onebot/​src/​main/​kotlin/​org/​foedusprogramme/​alexandrite/​channel/​onebot/​mapping/​OneBotMessages.kt:121

This blindly constructs a numeric MessageId, but this same mapper creates delivered refs named sent and async when no platform ID is available. Reusing either delivered ref as replyTo therefore throws before sending, rather than returning a delivery failure. Validate that the target is a numeric OneBot message ID and handle unavailable/synthetic refs explicitly.

Medium severity Preserve retryability when mapping HTTP failures

libraries/​channels/​onebot/​src/​main/​kotlin/​org/​foedusprogramme/​alexandrite/​channel/​onebot/​mapping/​OneBotMessages.kt:176

The mapping discards OneBotFailure.retryable for HTTP failures. In particular, an HTTP 5xx result is retryable by OneBotFailure, but it reaches Delivery.NotDelivered as non-retryable UNKNOWN, preventing normal retries. Pass the failure's retryability through (and classify retryable HTTP status failures as transient if appropriate).

Medium severity Do not use member card as group chat title

libraries/​channels/​onebot/​src/​main/​kotlin/​org/​foedusprogramme/​alexandrite/​channel/​onebot/​mapping/​OneBotMessages.kt:224

A group member's card is the sender's display name, not the group's title. Putting it into ChatInfo.title mislabels every group chat with whichever member spoke most recently. The event has no standard group-name field, so leave the title null unless it is obtained separately.

Medium severity Serialize typed request IDs using numeric ID serializers

libraries/​channels/​onebot/​src/​main/​kotlin/​org/​foedusprogramme/​alexandrite/​channel/​onebot/​protocol/​api/​OneBotRequests.kt:32

All typed request IDs are written as JSON strings here and in the remaining request classes, bypassing the ID serializers that deliberately emit a number when it fits (OneBotIds.kt:109-112). OneBot declares these parameters as numeric, so strict WS/JSON implementations can reject otherwise valid typed calls. Encode UserId, GroupId, and MessageId through their serializers (falling back to strings only when outside Long).

Medium severity Reject non-object JSON report elements

libraries/​channels/​onebot/​src/​main/​kotlin/​org/​foedusprogramme/​alexandrite/​channel/​onebot/​protocol/​event/​OneBotEventCodec.kt:26

A valid JSON scalar or array is converted into a synthetic Unknown event and therefore accepted by the HTTP receiver as a report, even though events must be objects; its original payload is also lost. Reject non-object elements (and have each transport classify/ignore that decode failure) instead of fabricating an event with zero IDs and empty raw JSON.

Medium severity Preserve GroupBan subType during decoding

libraries/​channels/​onebot/​src/​main/​kotlin/​org/​foedusprogramme/​alexandrite/​channel/​onebot/​protocol/​event/​OneBotEventCodec.kt:123

The codec computes subType but does not pass it to GroupBan, so both ban and lift_ban events expose subType == null despite the public model having that property. Preserve the wire value as the other notice decoders do.

Medium severity Omit null optional segment parameters from JSON

libraries/​channels/​onebot/​src/​main/​kotlin/​org/​foedusprogramme/​alexandrite/​channel/​onebot/​protocol/​message/​OneBotSegmentCodec.kt:79

Optional segment parameters are emitted as explicit JSON null values. explicitNulls = false does not remove JsonNull already inserted into a manually built object, so a simple Image(file = ...) sends null type, url, flags, and timeout instead of omitting optional fields as required by the request contract. Add each optional key only when non-null; the same pattern recurs in the record, video, poke, share, location, music, and node branches.

Medium severity Do not enqueue rejected reports as accepted

libraries/​channels/​onebot/​src/​main/​kotlin/​org/​foedusprogramme/​alexandrite/​channel/​onebot/​transport/​OneBotHttpPostReceiver.kt:171

Rejected reports have already been enqueued and counted as accepted before the callback's decision. This contradicts Rejected's contract and lets downstream processing act on a request answered with 4xx. Decide first, and only report/increment the accepted and quick-operation branches.

Medium severity Use plain text content type for rejection details

libraries/​channels/​onebot/​src/​main/​kotlin/​org/​foedusprogramme/​alexandrite/​channel/​onebot/​transport/​OneBotHttpPostReceiver.kt:195

Rejection bodies are plain text, but this labels every non-204 body as JSON. HTTP clients that honor the media type will try to parse text such as the signature... as JSON and fail to expose the actual reason. Use text/plain; charset=utf-8 for detail, retaining JSON only for quick-operation objects.

🧠 Review effort: Balanced

Yos-X added 3 commits October 10, 2026 23:16
…e drops

Five findings of the second review, all of them about a connection being
trusted for more than it is.

The HTTP receiver gave every accepted connection a thread of its own with no
bound and no limit on the whole request, so a client sending a byte at a time
held a thread for as long as it liked: `soTimeout` measures the gap between
two reads, not the request. The readers are now a bounded pool with a bounded
queue, a connection past that is answered 503 as busy instead of being held,
and the whole request carries a deadline that the head and the body are both
checked against.

The reverse listener read frames from every socket the library had upgraded,
not from the peer it serves, so a connection it had refused or one that a
newer peer replaced could report an event or answer a call of the connection
being served. Frames of a socket that is not the active peer are ignored now.
Its close was read the same way: a refused or superseded socket closing ended
every waiting call of the active connection, and now a close says nothing
about a connection that is not the one closing.

The count of dropped events was derived from the room the queue had, which
made it shrink as a reader caught up and reach zero once it had. The queue
names each event it never delivers, which is what is counted instead, so the
count only grows.

The header case test named its account in the body as well as the header, so
it was taken whichever of the two was read and passed even with the header
lookup broken. The body names no account now, which makes being taken the
header path working and nothing else.

Verified: :libraries:channels:onebot:test passes, 83 tests.
Findings of the second review that the first pass of this branch had not
read, taken from the review body rather than from its threads.

A report this instance refuses was put on the queue and counted as accepted
before the decision that refused it was taken, so a report the implementation
was told 4xx about reached whoever reads the events. The decision is taken
first now, and only an accepted or quick-operation report is reported and
counted.

Every body that was not 204 was answered as `application/json`, including the
sentence that says why a report was refused, which a client honouring the
type would fail to parse instead of showing the reason. A quick operation
stays JSON; a refusal is `text/plain; charset=utf-8`.

A notice of a group ban decoded `sub_type` into `isBan` alone and dropped it,
so a `ban` and a `lift_ban` were told apart but neither said which it was.
The wire value is kept, as the other notice decoders keep it.

A call over a socket threw where a call over HTTP answered: a socket that is
gone threw `IllegalStateException` and a call past its timeout threw
`TimeoutCancellationException`, so a channel sending a reply failed the turn
instead of reporting a delivery that did not happen. Both become
`OneBotResult.Unreachable`; a caller that gave up still travels as it is.

The mapping of a failure to a delivery read the kind of the failure and not
whether the same call could succeed again, so an answer of 5xx, which the
transport calls retryable, reached the caller as one that is not. It is asked
first now.

An optional parameter of a segment was written as a JSON null instead of
being left out, which the standard's request contract has no room for: an
implementation reading `type` as a string met a null. Every optional
parameter is written only when it is present.

Verified: :libraries:channels:onebot:test passes, 84 tests.
Two findings of the second review that the first pass of this branch had not
read.

A group message named the chat with the card of whoever sent it, which is
that member's own display name and not the name of the group: every group
would be labelled with the last person who spoke. A message carries no name
of its group, so the title is null until something asks for it.

A scalar or an array handed to the event decoder became an event that named
no time, no account and no kind, and its own payload was thrown away, so a
report that is no event was accepted as one worth reading. It is refused now,
which the transports already answer as a report they cannot read.

Verified: :libraries:channels:onebot:test passes, 84 tests.
@Yos-X

Yos-X commented Oct 10, 2026 •

Copy link
Copy Markdown
Collaborator Author

第二轮评审的处理结果。

按线程给出的 5 条发现,均已在 f1367d5 修复:接收器的读取线程改为有界线程池与排队位、超限以 503 拒绝,整请求另加截止时间(头部与正文的读取循环都检查);反向监听器只读取活动对端的帧,且只在关闭活动对端时才清理状态并结束等待中的调用;丢弃数改为在队列报告"从未交付"时递增;那条请求头用例改为正文不命名账号,使通过只能来自请求头路径。

正文中另一节"Previously missed"的 10 条也已逐条核实,全部修复:

  • 拒绝的报文先入队并计为已接受 —— b7b7346。决策先取,只有被接受或需要快速操作的报文才报告与计数。
  • 拒绝原因以 application/json 返回 —— b7b7346。拒绝为 text/plain; charset=utf-8,快速操作仍为 JSON。
  • GroupBan 丢失 sub_type —— b7b7346。
  • socket 调用抛出而非返回结果 —— b7b7346。连接断开与调用超时都转为 OneBotResult.Unreachable,调用方主动取消仍原样抛出。
  • 失败映射丢弃可重试性 —— b7b7346。先取传输给出的 retryable,5xx 因此成为可重试。
  • 可选段参数写成 JSON null —— b7b7346。仅在存在时写入。
  • 用成员名片作为群标题 —— 771f66d。消息不携带群名,标题保持为 null。
  • 非对象 JSON 被合成为事件 —— 771f66d。改为拒绝,传输层按无法读取的报文处理。
  • 回复目标直接构造数字 MessageId —— 3d 系列提交。合成引用 sent、async 不指向实现的任何消息,遇到时省略回复段并正常发出,而不是在发送前抛出。
  • 类型化请求的 id 以字符串写出 —— 同一提交。UserId、GroupId、MessageId 均经数字序列化器写出,超出 Long 时才回落为字符串。

验证::libraries:channels:onebot:test 通过,85 个测试。

…sage

Two findings of the second review that were left open.

A typed request wrote its ids as JSON strings, bypassing the serializers
that emit a number when the id fits in a `Long`, and the standard declares
these parameters numeric: a strict implementation is free to refuse a call
it would otherwise take. `UserId`, `GroupId` and `MessageId` now go through
the same numeric serializer the segments use, falling back to a string only
when the id is too large for one.

A reply named its target by constructing a numeric `MessageId` from whatever
the reference held, and this mapper is also what names a delivery `sent` or
`async` when the implementation answered with no id. Answering such a message
therefore threw before anything was sent, turning a lost thread into a lost
reply. A reference that names no message of the implementation is left out of
the answer's segments, so the answer is sent.

Verified: :libraries:channels:onebot:test passes, 85 tests.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Unresolved configuration, protocol, security, and WebSocket lifecycle defects can cause startup failure, data corruption, denial of service, and delayed calls.

3 open findings
5 resolved since last review
Previously missed (4)

In code that hasn't changed since last review

Medium severity Redact all non-empty configured secrets

libraries/​channels/​onebot/​src/​main/​kotlin/​org/​foedusprogramme/​alexandrite/​channel/​onebot/​auth/​OneBotAuth.kt:85

Values shorter than four characters are deliberately left unmasked, although access tokens and signing secrets have no minimum length. A non-2xx implementation response can echo such a token into OneBotFailure.message, violating this API's no-secret guarantee. Redact every non-empty configured value instead.

Medium severity Avoid using WebSocket echo as message ID

libraries/​channels/​onebot/​src/​main/​kotlin/​org/​foedusprogramme/​alexandrite/​channel/​onebot/​mapping/​OneBotMessages.kt:167

A socket response always carries this client's echo, but that value is only a correlation key, not the platform's message ID; toString() also includes JSON quotes for a string echo. When a successful send omits message_id, this therefore exposes a fabricated quoted ID instead of the documented sent fallback. Do not use echo as a message reference.

Medium severity Separate CQ text escaping from parameter escaping

libraries/​channels/​onebot/​src/​main/​kotlin/​org/​foedusprogramme/​alexandrite/​channel/​onebot/​protocol/​message/​CqCode.kt:174

CQ escaping treats plain text and parameter values differently: commas are escaped only inside parameter values. Because this shared function also encodes Text, ordinary text commas become non-standard &#44;; the matching decoder also corrupts a literal &#44; received in plain text. Split text escaping/unescaping from parameter escaping/unescaping.

Medium severity Fail pending calls when closing the forward WebSocket

libraries/​channels/​onebot/​src/​main/​kotlin/​org/​foedusprogramme/​alexandrite/​channel/​onebot/​transport/​OneBotForwardWebSocket.kt:72

Closing a forward WebSocket does not complete or clear client.awaiting. Cancelling the connection loop can prevent its post-onClose failPending call, so callers can remain suspended until firstByteTimeoutMillis after close() has returned. Atomically stop new sends and fail all pending deferreds before cancelling the scope.

🧠 Review effort: Balanced

Findings of the third review, which cover the listener, the sockets and the
credentials.

A configured value shorter than four characters was left in the clear, so a
short access token could be echoed back by an implementation and reach a
failure message. Nothing says a token is long, so every value that is not
empty is now replaced.

An answer over a socket carries the `echo` this client sent to match it to
its call, and this is not an id the implementation gave the message: a
delivery that named it invented one, quoted, where the documented fallback is
`sent`. The echo is no longer read as a name.

The declared body of a report over the limit was dropped without a deadline,
so a sender dripping it just faster than the idle timeout held a reader for
as long as it liked. The drain is under the same deadline as the request, and
its case now sends for longer than the deadline so that a listener without
one fails rather than passes.

Closing a forward connection cancelled the scope before the close handler
could end the calls that were waiting, so a caller stayed suspended past
`stop` until its own timeout. The waiting calls are ended first.

A reverse peer that replaced the one being served took over the connection
while the calls sent on the peer it replaced stayed in the map, waiting for
an answer that could no longer be read. Those calls are ended and the peer
they were sent on is closed before the newer one is served.

Escaping a comma was shared by plain text and by parameter values, but a
comma only ends a parameter: ordinary text had every comma turned into
`&#44;`, and a literal `&#44;` in text was read back as a comma. Text and
parameter values have their own escaping now, and the two cases that asserted
the old behaviour assert the new one.

Verified: :libraries:channels:onebot:test passes, 85 tests.
@Yos-X

Yos-X commented Oct 10, 2026

Copy link
Copy Markdown
Collaborator Author

第三轮评审的处理结果,全部已在 a4c5c32 修复。

按线程给出的 3 条:

  • 超限报文的排空路径绕过了整请求截止时间,未认证的发送方可以逐字节慢发占满读取线程 —— 已修复。drain 接收并检查同一个截止时间。
  • 新对端接管时,旧 socket 上等待中的调用被留在映射里,而其帧已不再读取,因此必然等到超时 —— 已修复。先结束这些调用并关闭旧 socket,再设为活动对端。
  • 该用例的发送时长(2 秒)短于截止时间(5 秒),因此没有截止时间也能通过 —— 已修复。改为发送 120 次共 12 秒,断言上界收紧到 8 秒。

正文中"Previously missed"的 4 条:

  • 短于四个字符的配置值未被脱敏,实现方回显该 token 时会进入失败信息 —— 已修复。凡非空值一律替换,token 与签名密钥没有最小长度。
  • 投递把 socket 应答的 echo 当作消息 id,而它只是本客户端用于配对的关联键,toString() 还会带上 JSON 引号 —— 已修复。应答未给出消息 id 时回落到 sent。
  • 正文与参数值共用一套转义,而逗号只在参数内表示结束,导致普通正文的逗号被写成 &#44;,正文里字面的 &#44; 又被读成逗号 —— 已修复。两者分开转义与反转义,原先断言旧行为的两条用例改为断言新行为。
  • 关闭正向 WebSocket 时,取消作用域可能先于关闭回调结束等待中的调用,close() 返回后调用方仍会挂起到超时 —— 已修复。等待中的调用先结束。

验证::libraries:channels:onebot:test 通过,85 个测试。20 条评审线程全部解决。

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants